Scrapbox: 指定したページが更新されても通知が来ないようにする #229

pizzacat83 · 2020-01-15T03:26:16Z

writeupなどネタバレを含むページの更新通知が #_scrapbox に流れてくるのがつらいので，指定したページは更新されても通知が来ないようにする

実装

記事が更新されるとScrapboxから指定したURLにWebhook向け形式のJSONが送られてくるので，送信先をSlackではなくTSGサーバーに設定してScrapboxから更新通知を受け取る。
TSGサーバーは更新されたページそれぞれが「ミュートしてほしいページ」かどうか判定し，そうでないもののみ更新通知を #_scrapbox に投稿する。

論点

Scrapbox通知をいじるのは割とTSGersに影響があると思うので，Slackbot民に限らず広く意見を募集します。

1. ミュートしたいページをどう指定するか

2. ミュートされたページの更新はどのように通知されるべきか

更新の内容は見たくなくても，更新されたという情報は欲しい場合がある

3. 誰が実装するか

4. ミュートタグのガイドライン

誰でもつけ外ししていいものなのか，それとも。

-1: 他に論点はないか

pizzacat83 · 2020-01-15T03:33:14Z

以下pizzacat83の構想です。

1.

ミュートしたいページのどこかに ###ミュート と書く
- # が3つあるのは他の記事との衝突を避けるため
サーバーは更新されたページのAPIを叩いてこのタグを含むかどうか調べる

2.

本文: 「この記事の更新通知はミュートされています。」
それ以外 (タイトルなど): そのまま

3.

私がやってもいいですが， @Szkieletor37 さんがやりたいとかだったらお譲りします。

Szkieletor37 · 2020-01-15T03:36:15Z

1,2 pizzacat氏の案に賛成します

3, 体調がアなため @pizzacat83 氏がやってくれるならありがたい

4, ミュートタグを個々の判断で勝手に入れてもいいのか、ガイドラインを決めて運用するのかを決める必要があると思います

pizzacat83 · 2020-01-15T03:41:53Z

4，論点に追加しました（ついでに4のindexがどんどんずれていくことに気がついたので-1にしました）

4はTSGerの良心に任せればまあええんちゃう〜と思っています。pizzacat83の2. なら更新自体の通知は来るため知らないうちにミュートということは起きないので，ミュートすべきかで人々の判断基準がずれていたらその時議論してくれればいいんじゃないかな〜（未来に丸投げ）

cookie-s · 2020-01-15T04:35:07Z

え〜実装方針案ふくめ構想もとてもよさそう〜楽しそう〜〜

1, む、###ミュートって含めるとどうなるんですか

4, TSGerはみんな良心の塊だからね！大丈夫なんちゃう！まぁでも「ガイドラインは今はありません、勝手にやってください、議論の対象になることはありますが咎められることはありません。」というガイドラインを書くべき説がある？

pizzacat83 · 2020-01-15T07:23:46Z

だいたい実装し終わったので，テストを書きます

codecov-io · 2020-01-15T07:25:17Z

Codecov Report

Merging #229 into master will increase coverage by 0.68%.
The diff coverage is 90.69%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   27.58%   28.26%   +0.68%     
==========================================
  Files          84       87       +3     
  Lines        6957     7029      +72     
  Branches     1343     1359      +16     
==========================================
+ Hits         1919     1987      +68     
- Misses       4393     4395       +2     
- Partials      645      647       +2

Impacted Files	Coverage Δ
index.js	`0% <ø> (ø)`	⬆️
welcome/index.ts	`0% <0%> (ø)`	⬆️
scrapbox/mute.ts	`100% <100%> (ø)`
lib/fastify.ts	`100% <100%> (ø)`
scrapbox/index.ts	`79.31% <87.5%> (-4.57%)`	⬇️
lib/scrapbox.ts	`90.47% <90.47%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb5f4a3...c3f17a0. Read the comment docs.

pizzacat83 · 2020-01-19T04:48:57Z

うおお結構時間かかってしまった:ojigineko-superfast:
テストは通っていて，手元でも動作確認してます。

2.

本文を上書きするのに加えて，画像とサムネを消すようにしました。

既存のコードの変更点

元からあったunfurlのコードと共通化できそうな部分は共通化しました。あと手元で動かすのが辛かったのでScrapboxのProject名は直書きせず環境変数を使うようにしました。

Merge時に必要な処理

サーバーに以下の環境変数を追加
- CHANNEL_SCRAPBOX: Cなんとか
- SCRAPBOX_PROJECT_NAME: tsg
Scrapboxの通知用URLをSlackのWebhookから https://TSGサーバーのURL/hooks/scrapbox に変更
- これはOwnerでないとできないっぽいのでよろしくお願いします

hideo54 · 2020-01-19T05:08:52Z

今更読んだんですけどめちゃくちゃ良さそう!!!! 今出てる案に全面的に同意です!

一つ提案なんですが、Slack 通知の時、アイコンを更新者のアイコンにしませんか?

hakatashi

実装ありがとう！大まかな仕様はよさそう

scrapbox/index.ts

scrapbox/index.test.ts

scrapbox/index.ts

pizzacat83 · 2020-03-04T14:38:16Z

scrapbox/index.test.ts

+			expect(attachments_res.length).toBe(
+				sum(separatedAttachments.map((a, i) => isMuted[i] ? 1 : a.length)),


attachments_res のテストが配列長だけで雑だなあとは思っている

scrapbox/index.test.ts

lib/scrapbox.ts

…notification

pizzacat83 · 2020-03-05T02:17:21Z

またしても時間がかかってしまった:ojigineko-superfast:
手元で動作確認済みです。

前回レビューからの変更点

ご指摘をいろいろ修正
エラーを握りつぶさないテスト用fastifyコンストラクタを lib/fastify.ts に配置
ScrapboxのAPIラッパを lib/scrapbox に作成
Scrapbox APIを使っていたScrapboxリンク展開とwelcomeがaxios直叩きから lib/scrapbox を使うように
scrapbox/index.ts(元からあったリンク展開) と scrapbox/mute.ts (今回実装した通知ミュート) に分割
- ファイルが大きくなってきたため
- npm run dev --only で片方だけ起動できるように
  - Private 個人プロジェクトで通知テストをしているときに他の部員がURL展開で個人プロジェクトを覗けてしまう
- Scrapbox周りのutilsを lib/ に移動したので共通して使うものが全て外部からのimportになったため
ミュートの仕様がかなり変わった (各記事APIコール -> ##ミュート のAPIコール, 画像大量崩壊notificationへの対応) ため，scrapbox/mute.ts とそのテストを大幅に再実装

前回レビュー時点での既存のコードの変更点

ScrapboxのProject名は直書きせず環境変数を使う
- 手元で動かすときに個人Scrapboxが使えるように

Merge時に必要な処理

サーバーに以下の環境変数を追加
- CHANNEL_SCRAPBOX: Cなんとか
- SCRAPBOX_PROJECT_NAME: tsg
Scrapboxの通知用URLをSlackのWebhookから https://TSGサーバーのURL/hooks/scrapbox に変更
- これはOwnerでないとできないっぽいのでよろしくお願いします

ほぼ0からの再レビューって感じだと思いますがよろしくお願いします:ojigineko:

hakatashi

鬼のように実装しててやべーーーーー開発力の塊???????????

いろいろコメントしたけど、大きな問題は無いと思います!

lib/fastify.ts

hakatashi · 2020-03-09T08:56:47Z

lib/fastify.ts

+     * 単体テストでexpectの失敗などによる例外をJestが検知することができない
+     * 発生した例外は全て再送出するように設定
+     */
+    fastify.setErrorHandler((err) => { throw err; });


[question] うーむ⋯⋯⋯⋯ fastify, ルートハンドラがthrowした場合はちゃんとinjectの結果をrejectしてくれると思ってたけど、どうだろう⋯⋯? (実際にこの行をコメントアウトしてもfastify.test.tsのテストが通る)

びっくりしているんですが，上にある throw Error をコメントアウトしても通っていて，少なくともテストコードはバグっています:ojigineko-superfast:

え〜この行無くても動くなどうして昔握りつぶされてInternal Server Errorしてたんだ？？？？？？

scrapbox/index.test.ts

scrapbox/mute.test.ts

hakatashi · 2020-03-09T09:30:58Z

lib/scrapbox.test.ts

+    });
+
+    describe('methods', () => {
+        let page: Page | null = null;


[question] strictNullChecksが入ってないので現状意味のない指定だと思いますが、大丈夫ですか

将来誰かがstrictNullChecksをしたくなった時用に lib/scrapbox.* はstrictNullChecksでも通るコードになってるんですが，どこかにその旨書いた方がいいですかね

なるほど、まあ意図があるなら大丈夫だけど、現状のプロジェクトの設定では検証できない制約をソースコードに課すのはあまり良くない気もする

welcome/index.ts

pizzacat83 · 2020-03-10T13:55:29Z

む前回テスト時からScrapbox通知の形式が変わった気がする調査します

cookie-s · 2020-11-07T00:00:52Z

これどうしたらいいですか、approveすればいいですか、すればいいならします

pizzacat83 · 2020-11-07T00:57:35Z

ぐえ私がレビューに対して修正をしていないという認識です…

pizzacat83 · 2021-01-20T13:08:30Z

無言で放置しておくのはよくないという気持ちになったので書いておきます

たぶん私はこのPRに手をつけません (100%とはいえないが)

タスクの残り:

PRのコメントへの対処
だいぶ時間が経ったので現在のScrapboxの通知フォーマットでも正常に動作するか確認する

hideo54 · 2021-01-20T13:21:56Z

そこそこのモチベーションあるので気が向いたら引き継ぎます。動き始める時はここで宣言しますね。(それまでに誰か開発したい人が現れたら譲ります)

scrapbox: dummy commit for draft pull request

3af7d47

pizzacat83 added 3 commits January 15, 2020 15:32

scrapbox: implement mute of notifications

72aec15

scrapbox: implement maskAttachment

6b48a20

scrapbox: type return value of maskAttachment

1bf5b2c

This comment has been minimized.

Sign in to view

scrapbox: implement isMuted

c7dd4dd

This comment has been minimized.

Sign in to view

scrapbox: don't show images

8a2141e

pizzacat83 self-assigned this Jan 15, 2020

pizzacat83 added 5 commits January 15, 2020 18:27

scrapbox: [WIP] add test for mute

5ca1d05

bcr: [Test Not Passing] mock axios

b6c6f6b

scrapbox: fix tests

4b93a9a

scrapbox: fix tests

e5a0d0e

scrapbox: update .env.sample

1444120

pizzacat83 requested review from cookie-s and hakatashi January 19, 2020 04:46

pizzacat83 marked this pull request as ready for review January 19, 2020 04:49

hakatashi requested changes Jan 19, 2020

View reviewed changes

scrapbox/index.ts Outdated Show resolved Hide resolved

scrapbox/index.ts Outdated Show resolved Hide resolved

scrapbox/index.ts Outdated Show resolved Hide resolved

scrapbox/index.test.ts Outdated Show resolved Hide resolved

cookie-s reviewed Jan 19, 2020

View reviewed changes

scrapbox/index.test.ts Outdated Show resolved Hide resolved

pizzacat83 commented Jan 19, 2020

View reviewed changes

scrapbox/index.test.ts Outdated Show resolved Hide resolved

pizzacat83 commented Jan 21, 2020

View reviewed changes

scrapbox/index.ts Outdated Show resolved Hide resolved

scrapbox: use (decode|encode)URIComponent

3c092fd

pizzacat83 added 4 commits March 4, 2020 10:01

lib/scrapbox: cache calls of getPageUrlRegExp

1244809

scrapbox: add helper class for tests

d3b9f5a

scrapbox: reimplement mute

c83d003

lib/scrapbox: fix unintended nullable

5f6b6ad

pizzacat83 commented Mar 4, 2020

View reviewed changes

pizzacat83 added 2 commits March 5, 2020 05:03

scrapbox: fix eslintrc

cbc5f2f

scrapbox: lint

93aadce

pizzacat83 commented Mar 4, 2020

View reviewed changes

lib/scrapbox.ts Outdated Show resolved Hide resolved

pizzacat83 added 5 commits March 5, 2020 07:08

lib/scrapbox: getPageUrlRegExp -> pageUrlRegExp and isPageOfProject

8ed67dc

scrapbox: add doc

6e16349

scrapbox: split files

2cead8b

Merge remote-tracking branch 'origin/master' into scrapbox-mute-some-…

a20913e

…notification

scrapbox/mute: add text: '' in image attachments

f435b6d

pizzacat83 requested review from cookie-s and hakatashi March 5, 2020 02:17

pizzacat83 mentioned this pull request Mar 5, 2020

Scrapbox通知でアイコンを更新者のアイコンに #257

Open

pizzacat83 added 2 commits March 9, 2020 10:31

lib/scrapbox: refactoring

2e717b0

scrapbox/mute: add comments

90e2ba8

hakatashi approved these changes Mar 9, 2020

View reviewed changes

pizzacat83 added 4 commits March 9, 2020 19:11

scrapbox: delete unnecessary mock

c2977de

scrapbox: remove dummy data not used in test

17642d4

scrapbox/mute: better variable name

2c20353

small refactoring

c3f17a0

pizzacat83 marked this pull request as draft January 20, 2021 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scrapbox: 指定したページが更新されても通知が来ないようにする #229

Scrapbox: 指定したページが更新されても通知が来ないようにする #229

pizzacat83 commented Jan 15, 2020 •

edited

Loading

pizzacat83 commented Jan 15, 2020

Szkieletor37 commented Jan 15, 2020

pizzacat83 commented Jan 15, 2020

cookie-s commented Jan 15, 2020 •

edited

Loading

This comment has been minimized.

pizzacat83 commented Jan 15, 2020

codecov-io commented Jan 15, 2020 •

edited by codecov bot

Loading

This comment has been minimized.

pizzacat83 commented Jan 19, 2020 •

edited

Loading

hideo54 commented Jan 19, 2020

hakatashi left a comment

pizzacat83 Mar 4, 2020

pizzacat83 commented Mar 5, 2020

hakatashi left a comment

hakatashi Mar 9, 2020

pizzacat83 Mar 9, 2020

pizzacat83 Mar 9, 2020

hakatashi Mar 9, 2020

pizzacat83 Mar 9, 2020

hakatashi Mar 9, 2020

pizzacat83 commented Mar 10, 2020

cookie-s commented Nov 7, 2020

pizzacat83 commented Nov 7, 2020

pizzacat83 commented Jan 20, 2021

hideo54 commented Jan 20, 2021

		expect(attachments_res.length).toBe(
		sum(separatedAttachments.map((a, i) => isMuted[i] ? 1 : a.length)),

Scrapbox: 指定したページが更新されても通知が来ないようにする #229

Are you sure you want to change the base?

Scrapbox: 指定したページが更新されても通知が来ないようにする #229

Conversation

pizzacat83 commented Jan 15, 2020 • edited Loading

実装

論点

1. ミュートしたいページをどう指定するか

2. ミュートされたページの更新はどのように通知されるべきか

3. 誰が実装するか

4. ミュートタグのガイドライン

-1: 他に論点はないか

pizzacat83 commented Jan 15, 2020

1.

2.

3.

Szkieletor37 commented Jan 15, 2020

pizzacat83 commented Jan 15, 2020

cookie-s commented Jan 15, 2020 • edited Loading

This comment has been minimized.

pizzacat83 commented Jan 15, 2020

codecov-io commented Jan 15, 2020 • edited by codecov bot Loading

Codecov Report

This comment has been minimized.

pizzacat83 commented Jan 19, 2020 • edited Loading

2.

既存のコードの変更点

Merge時に必要な処理

hideo54 commented Jan 19, 2020

hakatashi left a comment

Choose a reason for hiding this comment

pizzacat83 Mar 4, 2020

Choose a reason for hiding this comment

pizzacat83 commented Mar 5, 2020

前回レビューからの変更点

前回レビュー時点での既存のコードの変更点

Merge時に必要な処理

hakatashi left a comment

Choose a reason for hiding this comment

hakatashi Mar 9, 2020

Choose a reason for hiding this comment

pizzacat83 Mar 9, 2020

Choose a reason for hiding this comment

pizzacat83 Mar 9, 2020

Choose a reason for hiding this comment

hakatashi Mar 9, 2020

Choose a reason for hiding this comment

pizzacat83 Mar 9, 2020

Choose a reason for hiding this comment

hakatashi Mar 9, 2020

Choose a reason for hiding this comment

pizzacat83 commented Mar 10, 2020

cookie-s commented Nov 7, 2020

pizzacat83 commented Nov 7, 2020

pizzacat83 commented Jan 20, 2021

hideo54 commented Jan 20, 2021

pizzacat83 commented Jan 15, 2020 •

edited

Loading

cookie-s commented Jan 15, 2020 •

edited

Loading

codecov-io commented Jan 15, 2020 •

edited by codecov bot

Loading

pizzacat83 commented Jan 19, 2020 •

edited

Loading